Code Review

Overall, the feature is a nice UX improvement — adding the (View Report) link directly to the summary title is cleaner than hunting for it elsewhere. The dependency direction choice (HtmlReporter → GitHubReporter, not the reverse) is also sensible. A few concerns worth addressing:

🐛 Tip footer shows when HTML reporting is intentionally disabled

This is the main issue. The tip is gated solely on ArtifactUrl being null/empty, but that condition is true in multiple distinct situations:

1. TUNIT_DISABLE_HTML_REPORTER=true — MTP never calls HtmlReporter.OnTestSessionFinishingAsync, so ArtifactUrl is never set. The tip will tell users to "enable artifact upload" when they've intentionally disabled HTML reporting entirely.
2. No tests ran (_updates.Count == 0) — TryGitHubIntegrationAsync is never reached, same result.
3. Artifact upload was configured but failed (exception caught in TryGitHubIntegrationAsync) — the tip suggests "set it up" when the user already has it set up.

Only the "HTML reporter ran but runtime token is not available" case should show the tip.

Suggested fix: Make the tip opt-in via an explicit flag rather than inferring it from ArtifactUrl == null:

// GitHubReporter.cs
internal string? ArtifactUrl { get; set; }
internal bool ShowHtmlArtifactTip { get; set; }  // explicitly set by HtmlReporter

// GitHubReporter.AfterRunAsync — change the condition:
if (ShowHtmlArtifactTip)  // was: string.IsNullOrEmpty(ArtifactUrl)
{
    stringBuilder.AppendLine();
    stringBuilder.AppendLine("> **Tip:** ...");
}

// HtmlReporter.TryGitHubIntegrationAsync — signal the tip only when appropriate:
if (!hasRuntimeToken)
{
    if (_githubReporter is not null)
        _githubReporter.ShowHtmlArtifactTip = true;  // <-- explicit signal
    Console.WriteLine("Tip: To enable automatic HTML report artifact upload...");
}

This way the tip is only shown when HtmlReporter ran, reached GitHub integration, and found no runtime token — which is precisely when the tip is actionable.

Minor: The ArtifactUrl backdoor mutation

The internal string? ArtifactUrl { get; set; } exposed as a settable property is an implicit contract. The comment documents the MTP ordering assumption well, but the property is a public mutation point in disguise. A minor improvement would be to make it init-only or add a method to make the cross-component communication more intentional:

internal void SetArtifactUrl(string url) => ArtifactUrl = url;

This doesn't change semantics but signals "this is an intentional write from a peer component" vs. an accidental external mutation. Minor — take or leave it.

The temporal ordering is well-documented ✅

The comment // Set by HtmlReporter during OnTestSessionFinishingAsync, which MTP invokes before AfterRunAsync is exactly the right thing to say here. This is easy to miss and could bite someone during a refactor, so the comment earns its keep.

Summary: The feature concept is solid. The main thing to fix before merging is the false-positive tip scenario when HTML reporting is explicitly disabled or not running. The explicit ShowHtmlArtifactTip flag approach above resolves all three false-positive cases cleanly.

Follow-up Review

The main concern from the previous review has been addressed cleanly.

✅ False-positive tip — fixed

The second commit () introduced the explicit flag exactly as suggested. The tip is now only set to true inside the !hasRuntimeToken branch of TryGitHubIntegrationAsync, so it fires only when:

• We're running on GitHub Actions
• The HTML reporter ran and attempted integration
• No runtime token was found (i.e., artifact upload is not configured)

All three false-positive scenarios from the previous review are now correctly avoided.

Minor note (not blocking)

The internal string? ArtifactUrl { get; set; } / internal bool ShowArtifactUploadTip { get; set; } are still plain settable properties. The previous review flagged this as optional ("take or leave it"), and leaving it as-is is fine given that both are internal and the ordering is documented in comments.

Overall

The feature is clean, the dependency direction (HtmlReporter → GitHubReporter) is correct, and the main correctness issue is resolved. LGTM.

Follow-up Review

The main concern from the previous review has been addressed cleanly.

Fixed: False-positive tip

The second commit directly introduced the explicit ShowArtifactUploadTip flag exactly as suggested. The tip is now only set to true inside the !hasRuntimeToken branch of TryGitHubIntegrationAsync, so it fires only when:

• We are running on GitHub Actions
• The HTML reporter ran and attempted integration
• No runtime token was found (artifact upload is not configured)

All three false-positive scenarios from the previous review (HTML reporting disabled, no tests ran, upload configured but failed) are now correctly avoided.

Minor note (not blocking)

The internal string? ArtifactUrl { get; set; } and internal bool ShowArtifactUploadTip { get; set; } remain plain settable properties. The previous review flagged this as optional, and leaving it as-is is fine since both are internal and the MTP ordering assumption is documented in a comment.

Overall

The feature is clean, the dependency direction (HtmlReporter sets state on GitHubReporter, not the reverse) is correct, and the main correctness issue is resolved. LGTM.

Follow-up Review (final pass)

All concerns from previous reviews have been addressed. This is a summary review of the complete PR.

What the PR does

• Adds a (View Report) link to the GitHub summary table title when the HTML artifact upload succeeds.
• Shows a tip footer when artifact upload is not configured (instead of a vague null-based condition).
• Removes the now-redundant standalone View HTML Report step summary lines from HtmlReporter.

Previous review concerns — all resolved

False-positive tip (main issue, commit 2): The explicit ShowArtifactUploadTip flag is set only inside the !hasRuntimeToken branch of TryGitHubIntegrationAsync. This fires exclusively when the HTML reporter ran, reached GitHub integration, and found no runtime token — which is the only case where the tip is actionable. The three false-positive scenarios (HTML reporting disabled, no tests ran, upload configured but failed) are all correctly avoided.

Settable internal properties (minor, commit 3): Remains as plain internal settable properties. This is fine given they are internal and the MTP ordering assumption is documented in a comment. The refactor in commit 3 actually reduced surface area by removing the old summary-writing code from HtmlReporter entirely, which simplifies the overall design.

Code quality observations (no blocking issues)

• The temporal ordering dependency (OnTestSessionFinishingAsync before AfterRunAsync) is documented with a clear comment on the properties. Good.
• The dependency direction (HtmlReporter → GitHubReporter, not the reverse) is correctly maintained: GitHubReporter has no import or knowledge of HtmlReporter.
• SetGitHubReporter is called unconditionally before registration. If GitHubReporter.IsEnabledAsync() returns false (e.g., not in a GitHub Actions environment), the _githubReporter field in HtmlReporter is set but never written to from TryGitHubIntegrationAsync because the early-return guard if (Environment.GetEnvironmentVariable(EnvironmentConstants.GitHubActions) is not "true") return; fires first. So the coupling is harmless in non-GitHub environments.
• The URL format https://github.com/{repo}/actions/runs/{runId}/artifacts/{artifactId} is consistent with the old code and standard GitHub artifact URLs.

Overall

The PR is clean, correct, and the design is well-reasoned. LGTM.

Follow-up Review (latest commit 2e5b8c1)

One new commit landed after the last review, touching HtmlReporter.TryGitHubIntegrationAsync.

What changed

The _githubReporter interaction was restructured: rather than two separate null-checks (one inside the !hasRuntimeToken branch and one outside), there is now a single outer if (_githubReporter is not null) block with the !hasRuntimeToken / else if artifactId != null branching inside it.

Semantics

Logically identical to the previous version — the ShowArtifactUploadTip = true assignment still only fires when !hasRuntimeToken, and ArtifactUrl is still only set when upload succeeded. No regression.

Code quality

The restructuring is a minor improvement: one null check instead of two, and the full GitHubReporter state mutation is co-located in one place. Easier to read.

Overall

All concerns from previous reviews remain addressed. The PR is clean, correct, and ready to merge. LGTM.

Follow-up Review (commit 048fffc)

One new commit landed after the last review, addressing the two remaining issues.

Previous concerns — all resolved ✅

Wall-clock vs CPU time (main issue): Fixed. _runStopwatch = Stopwatch.StartNew() is now started in BeforeRunAsync and elapsed time is read in AfterRunAsync. The summary now correctly shows wall-clock elapsed time rather than the sum of individual test durations, which would have been wildly inflated for parallel test runs.

Artifact URL GHES compatibility: Fixed. _githubReporter.ArtifactUrl is now built using GITHUB_SERVER_URL with the https://github.com fallback, matching the approach used for source links. The .TrimEnd('/') guard prevents double-slash. GHES users will now get a correct "View Report" link.

Carry-forward nit: redundant null-bang operators in GetSourceLink

These were flagged in two prior reviews and are still present:

if (!string.IsNullOrEmpty(workspace) && filePath.StartsWith(workspace!, StringComparison.OrdinalIgnoreCase))
{
    filePath = filePath[workspace!.Length..].TrimStart('/');
}
// ...
var repoName = repo!.Split('/').LastOrDefault() ?? "";

The workspace! and repo! null-bangs are unnecessary — !string.IsNullOrEmpty() and the earlier string.IsNullOrEmpty(repo) null guard already prove non-null to the compiler. Not a bug, purely noise. Fine to leave if the PR has already had enough churn.

Overall

All substantive issues across the entire review history are now resolved. The PR is in good shape — wall-clock timing is correct, GHES links work end-to-end, the false-positive tip is prevented, and the UX output is significantly richer than before. LGTM.

Follow-up Review (commit 0822252)

One new commit landed after the last LGTM on `048fffc4`.

New commit: "fix: hide redundant segment breakdown when all tests pass" ✅

The if (passedCount != totalCount) guard is correct and well-motivated. When all tests pass, the top summary line (9 tests completed in 1.9s — 100.0% passed) already conveys everything the "✅ 9 passed" breakdown would repeat. Clean, focused change.

Carry-forward nit (from prior reviews): redundant null-bangs in GetSourceLink

Still present on lines 575, 577, 581 of GitHubReporter.cs:

filePath.StartsWith(workspace!, ...)   // workspace is proven non-null by the !IsNullOrEmpty check
filePath = filePath[workspace!.Length..]
var repoName = repo!.Split('/')        // repo is proven non-null by the earlier null guard

Not a bug — acknowledged in the previous review as noise. Fine to leave as-is.

New concern: unencoded HTML in <summary> and table cells

The existing HtmlReportGenerator.cs encodes all user-supplied content via WebUtility.HtmlEncode before inserting it into HTML. GitHubReporter does not follow this practice, and there are two spots where user-controlled content is embedded raw:

1. Inline failure <details> summary (line ~363):

stringBuilder.AppendLine($"<summary><code>{name}</code> ({duration}){sourcePart}</summary>");

name is $"{className}.{displayName}" where displayName is the MTP test node's display name, which for parametrized tests can include arbitrary string arguments. A parameter like "</code></summary><img src=x onerror=alert(1)>" would break the <summary> tag.

2. GetDetails → error <pre> (used in both inline details and the table):

return $"<pre>{GetError(stateProperty)}</pre>";

Exception messages are user-controlled (e.g., throw new Exception("</pre><b>injected</b>")), which can break out of the <pre> block.

Why it matters: GitHub does sanitize its Markdown-rendered step summaries (disallowing <script> etc.), but it permits many structural HTML tags. Breaking out of <details>/<summary> is likely safe from XSS in practice, but can corrupt the rendered summary layout in a confusing way.

Suggested fix — consistent with the existing HtmlReportGenerator pattern:

using System.Net;

// in the summary line:
$"<summary><code>{WebUtility.HtmlEncode(name)}</code> ({duration}){sourcePart}</summary>"

// in GetDetails:
return $"<pre>{WebUtility.HtmlEncode(GetError(stateProperty))}</pre>";

// in table cells:
detailsBuilder.AppendLine($"<td>{WebUtility.HtmlEncode(name)}</td>");
detailsBuilder.AppendLine($"<td>{WebUtility.HtmlEncode(details)}</td>");

Note: status and duration are reporter-generated strings (not user input), so they don't need encoding.

Overall

The new commit is clean and correct. The one substantive concern worth addressing is the missing HtmlEncode on user-supplied content that gets embedded in HTML tags — especially given that the codebase already has the right pattern in HtmlReportGenerator. The null-bang nit remains a carry-forward non-issue.

Follow-up Review (latest state)

All major issues from previous review rounds have been addressed. This is a comprehensive pass over the full PR as merged.

✅ All prior concerns resolved

• False-positive tip — fixed via flag
• Wall-clock vs CPU time — fixed via in
• FormatDuration hours rounding — fixed via (int) cast instead of :F0
• Source link path double-stripping — fixed; is primary, repo-name fallback is secondary
• GHES compatibility — fixed via env var with fallback
• Segment breakdown on all-pass — fixed via guard

Remaining nit: redundant early return

The if (passedCount == last.Count) early return (around line 276 in the new file) is still present. With the new structure, when all tests pass:

• failureMessages and otherMessages will both be empty after the loop
• All subsequent if (failureMessages.Count > 0) and if (hasRemainingDetails) blocks are skipped
• The final --- separator is written by both code paths

The early return and the unified path produce identical output. It's not a bug, just slightly redundant. If you ever want to clean it up, you can remove it and rely on the conditional sections below it.

Minor: flaky test timing shows last-attempt duration only

flakyTests.Add((name, finalStateCount, timing?.GlobalTiming.Duration));

The timing is taken from the last update (the successful attempt). For a test that timed out twice before passing, the summary would show something like 3 attempts (0.8s) — the 0.8s is the final fast pass, not the total. This could feel misleading when debugging flaky tests.

Suggested improvement: Sum durations across all attempts in _updates[kvp.Key], or at minimum append a note like "(final attempt)". This is low priority but worth a future issue.

Minor: HTML in test names / error messages

Test names are embedded in <summary><code>{name}</code>... and error details are written into table <td> cells without HTML-escaping. A test named or error message containing </code></summary></details> could break the summary structure. This is pre-existing (the old table had the same issue) and low-probability since test names come from code, but System.Net.WebUtility.HtmlEncode on names and detail strings would be strictly safer.

Overall

This is a well-executed feature PR. The GitHub step summary output goes from a minimal table to a genuinely useful CI artifact: status at a glance, flaky detection, skipped grouping, inline failure details with source links, and human-readable timing. The architecture (HtmlReporter pushes to GitHubReporter, not vice versa) is the right call for keeping concerns separated without over-engineering it.

The two minor points above are non-blocking suggestions for future work — they don't affect correctness today. LGTM.

Follow-up Review (latest state)

All major issues from previous review rounds have been addressed. This is a comprehensive pass over the full PR as merged.

All prior concerns resolved

• False-positive tip — fixed via ShowArtifactUploadTip flag
• Wall-clock vs CPU time — fixed via Stopwatch in BeforeRunAsync
• FormatDuration hours rounding — fixed via (int) cast instead of :F0
• Source link path double-stripping — fixed; GITHUB_WORKSPACE is primary, repo-name fallback is secondary
• GHES compatibility — fixed via GITHUB_SERVER_URL env var with fallback
• Segment breakdown on all-pass — fixed via passedCount != totalCount guard

Remaining nit: redundant early return

The if (passedCount == last.Count) early return (around line 276 in the new file) is still present. With the new structure, when all tests pass:

• failureMessages and otherMessages will both be empty after the loop
• All subsequent if (failureMessages.Count > 0) and if (hasRemainingDetails) blocks are skipped
• The final --- separator is written by both code paths

The early return and the unified path produce identical output. Not a bug, just slightly redundant. Could be removed to rely on the conditional sections below it.

Minor: flaky test timing shows last-attempt duration only

flakyTests.Add((name, finalStateCount, timing?.GlobalTiming.Duration));

The timing is taken from the last update (the successful attempt). For a test that timed out twice before passing, the summary would show something like 3 attempts (0.8s) — the 0.8s is the final fast pass, not the total. This could feel misleading when debugging flaky tests.

Suggested improvement: Sum durations across all attempts in _updates[kvp.Key], or at minimum append a label like "(final attempt)". Low priority but worth a future issue.

Minor: HTML in test names / error messages (pre-existing)

Test names are embedded in <summary><code>{name}</code>... and error details go into table <td> cells without HTML-escaping. A test name or error message containing </code></summary></details> could break the summary structure. This is a pre-existing issue (old table had the same problem) and low-probability since test names come from code, but System.Net.WebUtility.HtmlEncode on names and detail strings would be strictly safer.

Overall

This is a well-executed feature. The GitHub step summary goes from a minimal table to a genuinely useful CI artifact: status at a glance, flaky detection, skipped grouping, inline failure details with source links, and human-readable timing. The architecture (HtmlReporter pushes to GitHubReporter, not vice versa) is the right call for keeping concerns separated without over-engineering.

The two minor points above are non-blocking suggestions — they don't affect correctness. LGTM.